Skip to content

Fix get_filename_list crash when a cached model folder is deleted#14497

Open
JSap0914 wants to merge 1 commit into
Comfy-Org:masterfrom
JSap0914:fix-filename-list-cache-deleted-folder
Open

Fix get_filename_list crash when a cached model folder is deleted#14497
JSap0914 wants to merge 1 commit into
Comfy-Org:masterfrom
JSap0914:fix-filename-list-cache-deleted-folder

Conversation

@JSap0914

Copy link
Copy Markdown

Bug

get_filename_list() crashes with FileNotFoundError when a model
folder that was present at cache-build time is deleted while ComfyUI is
running.

cached_filename_list_() validates the cache by comparing the stored
mtime of every directory recorded during the previous scan
(recursive_search records each subfolder's mtime, not just the
top-level roots). It does this with a bare os.path.getmtime(folder).
If any tracked folder has since been removed, getmtime() raises
FileNotFoundError, which propagates out of get_filename_list() and
breaks model listing for that category — instead of just invalidating
the stale cache and rebuilding it.

Repro:

import os, shutil, tempfile, folder_paths

base = tempfile.mkdtemp()
cat = os.path.join(base, "category"); sub = os.path.join(cat, "sub")
os.makedirs(sub)
open(os.path.join(cat, "a.safetensors"), "w").close()
open(os.path.join(sub, "b.safetensors"), "w").close()
folder_paths.folder_names_and_paths["demo"] = ([cat], {".safetensors"})

folder_paths.get_filename_list("demo")   # builds cache, records sub/ mtime
shutil.rmtree(sub)                         # folder removed at runtime
folder_paths.get_filename_list("demo")   # -> FileNotFoundError

Fix

Treat an inaccessible tracked folder as a stale-cache signal: catch
OSError around the getmtime probe and return None, so the cache is
rebuilt rather than raising. This mirrors the existing behaviour when a
folder's mtime simply changed.

Verification

$ python -m pytest tests-unit/folder_paths_test/ -v
...
35 passed in 0.14s

The suite includes a new regression test
(tests-unit/folder_paths_test/cache_invalidation_test.py) that builds
the cache, deletes a tracked subfolder, and asserts the next
get_filename_list() call rebuilds successfully instead of crashing. It
fails on master and passes with this change.

cached_filename_list_ probes os.path.getmtime() for every directory
recorded while the filename cache was built, including subfolders. If
one of those folders is removed at runtime (e.g. the user deletes a
model folder), getmtime() raises FileNotFoundError, which propagates
out of get_filename_list() and breaks model listing instead of simply
rebuilding the cache.

Treat an inaccessible tracked folder as a stale-cache signal and return
None so the list is rebuilt.
Copilot AI review requested due to automatic review settings June 16, 2026 03:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes folder_paths filename cache invalidation when a previously tracked subfolder is deleted, and adds a regression test to ensure the cache rebuilds instead of raising during mtime checks.

Changes:

  • Treat missing/inaccessible tracked folders as a stale cache condition during cache validation.
  • Add a unit test that deletes a tracked subfolder and verifies get_filename_list() rebuilds correctly.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
tests-unit/folder_paths_test/cache_invalidation_test.py Adds regression coverage for deleting a tracked subfolder after cache population.
folder_paths.py Updates cache validation to handle deleted/inaccessible tracked folders without raising.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pytest.fixture
def model_folder():
"""Register a temporary model category with a tracked subfolder."""
folder_name = "cache_invalidation_test_cat"
open(os.path.join(category, "a.safetensors"), "w").close()
open(os.path.join(subfolder, "b.safetensors"), "w").close()

folder_paths.folder_names_and_paths[folder_name] = (
Comment on lines +28 to +30
folder_paths.folder_names_and_paths.pop(folder_name, None)
folder_paths.filename_list_cache.pop(folder_name, None)
folder_paths.cache_helper.clear()
Comment on lines +45 to +46
refreshed = folder_paths.get_filename_list(folder_name)
assert refreshed == ["a.safetensors"]
Comment thread folder_paths.py
Comment on lines +415 to 421
try:
if os.path.getmtime(folder) != time_modified:
return None
except OSError:
# A tracked folder was deleted or became inaccessible; treat the
# cache as stale so it gets rebuilt instead of raising.
return None
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a1aa4b96-7f83-4642-9100-3deffa32ccf9

📥 Commits

Reviewing files that changed from the base of the PR and between 5db51b7 and fe89d39.

📒 Files selected for processing (2)
  • folder_paths.py
  • tests-unit/folder_paths_test/cache_invalidation_test.py

📝 Walkthrough

Walkthrough

cached_filename_list_ in folder_paths.py now wraps its per-folder os.path.getmtime comparison in a try/except OSError. When a tracked folder has been deleted or becomes inaccessible, the function returns None to signal a stale cache instead of allowing the error to propagate. A new test file adds a model_folder pytest fixture that registers a temporary tracked subfolder and cleans up afterward, plus a regression test that primes the cache, deletes the subfolder, and confirms the next get_filename_list call rebuilds cleanly.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: preventing a crash in get_filename_list when a cached model folder is deleted.
Description check ✅ Passed The description comprehensively explains the bug, the fix, and includes verification with a reproduction case and test results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants